Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding required structs and methods to get a list of publishers or subscribers with their respective qos #186

Merged

Conversation

jaisontj
Copy link
Contributor

@jaisontj jaisontj commented Sep 27, 2019

Includes creating relevant data structures to enable the feature mentioned here. Relevant information about implementation is discussed here

Summary:

  • Added two new structs
    • rmw_participant_qos_profile_t to store participant name and their qos policy in one struct
    • rmw_participants_t to store a list of rmw_participant_qos_profile_t
  • Added two new functions:
    • rmw_participant_qos_profile_allocate
    • rmw_participant_qos_profile_free
  • Tests for the above two functions

Note: colcon build --packages-up-to rmw && colcon test --packages-up-to rmw && colcon test-result --verbose --test-result-base build/rcl passes without any errors or failures.

@jaisontj jaisontj force-pushed the jaisontj/include_qos_in_ros2_topic_info branch 2 times, most recently from b05810b to 1610f36 Compare September 28, 2019 00:11
@jaisontj jaisontj marked this pull request as ready for review September 30, 2019 15:43
Copy link

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test

rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
@jaisontj jaisontj changed the title jaisontj/include qos in ros2 topic info jaisontj/adding required structs and methods to get a list of publishers/subscribers with their respective qos Oct 1, 2019
@jaisontj jaisontj force-pushed the jaisontj/include_qos_in_ros2_topic_info branch from 1610f36 to f138591 Compare October 1, 2019 17:24
@jaisontj jaisontj changed the title jaisontj/adding required structs and methods to get a list of publishers/subscribers with their respective qos Adding required structs and methods to get a list of publishers or subscribers with their respective qos Oct 4, 2019
@jaisontj jaisontj force-pushed the jaisontj/include_qos_in_ros2_topic_info branch from 49af797 to 36e4a68 Compare October 4, 2019 01:20
@jaisontj jaisontj force-pushed the jaisontj/include_qos_in_ros2_topic_info branch from 25d4236 to f3b5b72 Compare October 8, 2019 00:58
Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why rmw_get_qos_for_publishers and rmw_get_qos_for_subscribers return a list of rmw_qos_profile_t (with the participant name), instead of just returning one.

IMO, both of them should return only one rmw_qos_profile_t, using the data discovered by the rmw_node_t passed as an argument.

rmw/include/rmw/allocators.h Outdated Show resolved Hide resolved
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Member

ivanpauno commented Oct 14, 2019

I don't get why rmw_get_qos_for_publishers and rmw_get_qos_for_subscribers return a list of rmw_qos_profile_t (with the participant name), instead of just returning one.

IMO, both of them should return only one rmw_qos_profile_t, using the data discovered by the rmw_node_t passed as an argument.

Sorry, now I see why you need a list (there may be many publishers/subscription on each topic, in different nodes).
Please, always refer to the node name and to the node.
Participant and its name are not a concept in ROS 2.

@jaisontj
Copy link
Contributor Author

jaisontj commented Oct 14, 2019

I don't get why rmw_get_qos_for_publishers and rmw_get_qos_for_subscribers return a list of rmw_qos_profile_t (with the participant name), instead of just returning one.
IMO, both of them should return only one rmw_qos_profile_t, using the data discovered by the rmw_node_t passed as an argument.

Sorry, now I see why you need a list (there may be many publishers/subscription on each topic, in different nodes).
Please, always refer to the node name or node.
Participant and its name are not a concept is ROS 2.

  • I used Participant as a blanket term for Publishers and Subscribers. Since rmw_participant_qos_profile_t and rmw_partiticpants_t can be used for both publishers and subscribers.
  • Since each topic can have multiple publishers/subscribers, the participant_name (aka publisher_name or subscriber_name) is important to differentiate between them when displaying from ros2cli

For eg: the command ros2 topic info my_topic should print

Topic: my_topic

Publisher count: 2

Publisher Name: _ros2cli_daemon_0
QoS Profile: 
    Reliability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
    Durability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
    Lifespan: 1000000000 nanoseconds
    Deadline: 2000000000 nanoseconds
    Liveness: RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC
    Liveness Lease Duration: 50000 nanoseconds

Publisher Name: talker
QoS Profile: 
    Reliability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
    Durability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
    Lifespan: 1000000000 nanoseconds
    Deadline: 2000000000 nanoseconds
    Liveness: RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC
    Liveness Lease Duration: 5 nanoseconds

Subscriber count: 0

What do you suggest is a better term to Participants as I see that Participants can be misleading?

@ivanpauno
Copy link
Member

Topic: my_topic

Publisher count: 2

Publisher Name: _ros2cli_daemon_0
QoS Profile:
Reliability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
Durability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
Lifespan: 1000000000 nanoseconds
Deadline: 2000000000 nanoseconds
Liveness: RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC
Liveness Lease Duration: 50000 nanoseconds

Publisher Name: talker
QoS Profile:
Reliability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
Durability: RMW_QOS_POLICY_RELIABILITY_RELIABLE
Lifespan: 1000000000 nanoseconds
Deadline: 2000000000 nanoseconds
Liveness: RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC
Liveness Lease Duration: 5 nanoseconds

Subscriber count: 0

It should say node name, Publishers or Subscriptions don't have a name (the name you're printing is the node name).
You can also create many publishers/subscriptions in a node in the same topic (and with different qos settings, or not).
When that happens, you don't have a way to distinguish between one publisher/subscription and the other.
It's a strange use case, but it's possible.

@jaisontj
Copy link
Contributor Author

jaisontj commented Oct 14, 2019

It should say node name, Publishers or Subscriptions don't have a name (the name you're printing is the node name).
You can also create many publishers/subscriptions in a node in the same topic (and with different qos settings, or not).
When that happens, you don't have a way to distinguish between one publisher/subscription and the other.
It's a strange use case, but it's possible.

Ah, I see what you mean.

Alright, so participant_name can be renamed to node_name.
Whats your take on renaming rmw_participants_t and rmw_participant_qos_profile_t? Since rmw_nodes_t might be misleading since we already have rmw_node_t. The same goes for rmw_node_qos_profile_t. Ideally, an umbrella term for publishers and subscribers is what we want.

@ivanpauno
Copy link
Member

Whats your take on renaming rmw_participants_t and rmw_participant_qos_profile_t? Since rmw_nodes_t might be misleading since we already have rmw_node_t. The same goes for rmw_node_qos_profile_t. Ideally, an umbrella term for publishers and subscribers is what we want.

We currently have two types of graph introspection functions:

I think that getting the list of publishers and subscribers with its qos enters in the second group, we could have a *_by_node function.
In that case, it's not necessary to retrieve the node name in the output, as it was part of the arguments. We could just add a rmw_qos_profiles_t, which is a list of the current rmw_qos_profile_t.

Does that sound reasonable?

@jaisontj
Copy link
Contributor Author

I think that getting the list of publishers and subscribers with its qos enters in the second group, we could have a *_by_node function.
In that case, it's not necessary to retrieve the node name in the output, as it was part of the arguments. We could just add a rmw_qos_profiles_t, which is a list of the current rmw_qos_profile_t.

I am a little confused. The only data available is the handle to rmw_node_t and the topic_name, with this we need to find all publishers and subscribers (along with their QoS Policies) to that topic_name. In this case aren't we also getting publishers and subscribers from other nodes as well?
How can node name not be part of the arguments when we do not know of the other nodes involved?

@ivanpauno
Copy link
Member

@jaisontj I'm thinking in something like this:

rmw_get_publisher_qos_by_node(
  const rmw_node_t * node,
  rcutils_allocator_t * allocator,
  const char * node_name,
  const char * node_namespace,
  rmw_qos_profiles_t * qos_profiles);

I think that matches better the current API.

And actually, I would go with a method like this:

rmw_get_publisher_names_types_and_qos_by_node(
  const rmw_node_t * node,
  rcutils_allocator_t * allocator,
  const char * node_name,
  const char * node_namespace,
  bool demangle,
  rmw_names_and_types_t * topic_names_and_types,
  rmw_qos_profiles_t * qos_profiles);

rmw_names_and_types_t and rmw_qos_profiles_t will have in the same index info of the same publisher (it's not possible to ensure that if it's separated in two calls).


If you want to go with something similar to your first proposal, I would rather go with:

rmw_ret_t
rmw_get_qos_for_publishers(
  const rmw_node_t * node,
  const char * topic_name,
  const char * type_name,
  bool demangle,
  rmw_node_names_t * node_names,
  rmw_qos_profiles_t * qos_profiles);

I added type_name, as you probably also want to know it.
demangle option is taken by all graph API functions, it specifies if ROS topic and type name conventions are used or not.
rmw_node_names_t would be a list of node names (and namespaces).
rmw_qos_profiles_t will have the same indexing as rmw_node_names_t.

@jaisontj
Copy link
Contributor Author

Thank you for this @ivanpauno. After looking at your suggestions, I have slight modifications in mind

 rmw_get_publisher_names_and_qos_for_topic(
   const rmw_node_t * node,
   rcutils_allocator_t * allocator,
   const char * topic_name,
   rcutils_string_array_t * node_names,
   rmw_qos_profiles_t * qos_profiles);

I've renamed the function from *_by_node to *_for_topic since we are getting a list of all publishers to a topic, regardless of which node it is a part of.

Parameters:

  • allocator is required to allocate memory for node_names and qos_profiles since we do not know the size of the array before hand.
  • topic_name: the topic name to get the list of publishers for; this must be a fully qualified topic_name
  • node_names and qos_profiles: which have the same indexing for participants

I've taken off:

  • type_name as I'm not sure why we need it. We can get all the data we need from the topic_name and rmw_node_t (from the data inside topic_cache). Moreover, when this call originates from the cli, the command would be ros2 topic info <topic_name>.
  • demangle: similar to how rmw_count_publisher does it, could we not specify that the topic name must be fully qualified; like here (linking to rcl as the rmw package has no comments/documentation for the functions)?

Let me know if I've gotten this right. If so, I will update this PR with the same :)

@emersonknapp
Copy link
Contributor

emersonknapp commented Oct 14, 2019

@ivanpauno I think some of the confusion here is that we are querying for information for everything except this node. There isn't a precedent in the graph API of "information_by_topic", but we need that info for this new functionality.

@jaisontj For naming consistency, it may make more sense to name it rmw_get_qos_profiles_by_topic since it seems the _by_X naming has precedent, whereas the _for_X doesn't.

Note: the name convention in ROS2 is subscription not subscriber. There is no subscriber in ROS2. Unfortunately it seems that a handful of functions that mention subscriber have made it into rmw. Lets make sure not to continue the trend.

So to summarize that, I think you're on the right track and we're looking at

rmw_get_publisher_names_and_qos_by_topic
rmw_get_subscription_names_and_qos_by_topic

@ivanpauno does that sound about right?

EDIT: more clarification - the purpose of this code change is to gain information about the rest of the running system, with the goal of later exposing QoS mismatch when we try to subscribe to a topic whose publisher has an incompatible QoS to our subscription.

@mm318
Copy link
Member

mm318 commented Jan 7, 2020

Even though ros2/rclcpp#960 (which supersedes ros2/rclcpp#959) is out for review, are we we ready to merge the "rcl and below" subset now?

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good to me, there was only one thing that I think must be fixed (related to not checking if allocate fails in one case), but there were lots of other comments that may or may not need to be addressed.

rmw/include/rmw/get_topic_info.h Outdated Show resolved Hide resolved
rmw/include/rmw/get_topic_info.h Outdated Show resolved Hide resolved
rmw/include/rmw/get_topic_info.h Outdated Show resolved Hide resolved
* The topic_name parameter must not be `NULL` and must follow the topic naming rules
* mentioned at http://design.ros2.org/articles/topic_and_service_names.html
* Non existent topic names are allowed.
* In that case, this function will return an empty array.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like they are allowed? Why not an error case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what error code to use in this case (a generic RMW_RET_ERROR? RCL_RET_TOPIC_NAME_INVALID migrated over to rmw?).

Also, this function shouldn't be used for detecting the existence of a topic; that feels like it is encroaching on the purpose of rmw_get_topic_names_and_types().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this comment doesn't make since if "non-existent topic" just means a topic that isn't on the graph. I was still thinking an empty string, in which case it should create an error, just like if it violated any of the other rules in the topic name rules set.

I think this can be resolved without changes. Though a follow up question is what happens if the value for topic_name violates one of the topic name rules, e.g. empty string, starts with a number, etc.

There is already this function:

/// Determine if a given fully qualified topic name is valid.
/** Validity of a FQN for topic is determined based on rules defined here:
*
* http://design.ros2.org/articles/topic_and_service_names.html
*
* Note that this function expects any URL suffixes as described in the above
* document to have already been removed.
*
* If either the C string or validation_result pointer are null, then
* `RMW_RET_INVALID_ARGUMENT` will be returned.
* The topic_name should be a valid, null-terminated C string.
* The validation_result int pointer should point to valid memory so a result
* can be stored in it as an output variable.
* The invalid_index size_t pointer should either point NULL or to valid memory
* so in the event of a validation error, the location in the input string can
* be stored therein.
* If NULL is passed in for invalid_index, it will be not be set.
*
* The invalid_index will not be assigned a value if the topic is valid.
*
* The int which validation_result points to will have a one of a few possible
* results values (defined with macros) stored into it:
*
* - RMW_TOPIC_VALID
* - RMW_TOPIC_INVALID_IS_EMPTY_STRING
* - RMW_TOPIC_INVALID_NOT_ABSOLUTE
* - RMW_TOPIC_INVALID_ENDS_WITH_FORWARD_SLASH
* - RMW_TOPIC_INVALID_CONTAINS_UNALLOWED_CHARACTERS
* - RMW_TOPIC_INVALID_CONTAINS_REPEATED_FORWARD_SLASH
* - RMW_TOPIC_INVALID_NAME_TOKEN_STARTS_WITH_NUMBER
* - RMW_TOPIC_INVALID_TOO_LONG
*
* The result value can be converted to a description with the
* rmw_full_topic_name_validation_result_string() function.
*
* The `RMW_TOPIC_INVALID_TOO_LONG` is guaranteed to be checked last, such
* that if you get that result, then you can assume all other checks succeeded.
* This is done so that the length limit can be treated as a warning rather
* than an error if desired.
*
* \param[in] topic_name topic name to be validated
* \param[out] validation_result int in which the result of the check is stored
* \param[out] invalid_index size_t index of the input string where an error occurred
* \returns `RMW_RET_OK` on successfully running the check, or
* \returns `RMW_RET_INVALID_ARGUMENT` on invalid parameters, or
* \returns `RMW_RET_ERROR` when an unspecified error occurs.
*/
RMW_PUBLIC
RMW_WARN_UNUSED
rmw_ret_t
rmw_validate_full_topic_name(
const char * topic_name,
int * validation_result,
size_t * invalid_index);

rmw/include/rmw/get_topic_info.h Outdated Show resolved Hide resolved
rmw/include/rmw/topic_info.h Outdated Show resolved Hide resolved
rmw/include/rmw/topic_info.h Outdated Show resolved Hide resolved
rmw/src/topic_info.c Outdated Show resolved Hide resolved
rmw/src/topic_info.c Outdated Show resolved Hide resolved
rmw/src/topic_info_array.c Outdated Show resolved Hide resolved
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
Signed-off-by: Miaofei <miaofei@amazon.com>
@ivanpauno
Copy link
Member

I see rcl and downstream PRs ready for another round of CI, @wjwwood what do you think?

@ivanpauno
Copy link
Member

ivanpauno commented Jan 13, 2020

Just rmw_fastrtps_cpp:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
    Including all the stubs:
  • Windows Build Status

@ivanpauno
Copy link
Member

@mm318 Can you check the macOS compiler warnings? https://ci.ros2.org/job/ci_osx/7362/warnings11Result/

@mm318
Copy link
Member

mm318 commented Jan 13, 2020

Hmm, interesting this post here and these examples in rcl here and here seem to say these warnings would be false positives with GCC, and would not happen with Clang in the ROS2 codebase.

Anyway, I'll implement the solution found here: https://stackoverflow.com/a/9293087

Signed-off-by: Miaofei <miaofei@amazon.com>
@ivanpauno
Copy link
Member

ivanpauno commented Jan 14, 2020

Hmm, interesting this post here and these examples in rcl here and here seem to say these warnings would be false positives with GCC, and would not happen with Clang in the ROS2 codebase.

Anyway, I'll implement the solution found here: https://stackoverflow.com/a/9293087

That's strange, ignoring the warning sounds reasonable.

@ivanpauno
Copy link
Member

  • macOS Build Status

@ivanpauno ivanpauno merged commit a377453 into ros2:master Jan 14, 2020
@ivanpauno
Copy link
Member

Merged!! Thanks for your work @mm318 @jaisontj !!

@mm318
Copy link
Member

mm318 commented Jan 14, 2020

Thank you for merging, @ivanpauno! Let's continue the conversation at ros2/rclcpp#960.

@jaisontj
Copy link
Contributor Author

Thank you @mm318 for following up on these :)

wjwwood added a commit that referenced this pull request Jan 27, 2020
wjwwood added a commit that referenced this pull request Jan 27, 2020
@thomas-moulard thomas-moulard mentioned this pull request Mar 4, 2020
3 tasks
Karsten1987 added a commit that referenced this pull request Mar 26, 2020
#186 introduces two functions to the RMW interface which are not present when including `rmw/rmw.h`.
For convenience to the rmw implementer, I think it makes sense to include all functions when including `rmw/rmw.h`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.